feat(assembly): add Docker image build support via Maven profile (#1701)#1757
feat(assembly): add Docker image build support via Maven profile (#1701)#1757jbonofre merged 4 commits intoapache:mainfrom
Conversation
…lly", another to publish on DockerHub with multi-platforms (apache#1701)
jeanouii
left a comment
jeanouii
left a comment
There was a problem hiding this comment.
LGTM
Small comments regarding the plugin versions and the goal=push for the profile.
Wondering if it's meant to replace the previous script and therefor if we should have the script deleted along side these changes to address the issue.
The dockerfile is out side this PR scope, but it would be great to pass in the Java version instead of having it hard coded (so we can use 17/21 or 25).
In terms of versioning, we probably also need latest to be created and have the git tag somewhere.
High level thoughts. hopefully it helps
assembly/pom.xml
Outdated
| <plugin> | ||
| <groupId>io.fabric8</groupId> | ||
| <artifactId>docker-maven-plugin</artifactId> | ||
| <version>0.45.1</version> |
There was a problem hiding this comment.
We use the version twice. Not a blocker, but would be better to extract it and reuse it.
There was a problem hiding this comment.
Also do we need goal -> push?
The docker images are built locally but never pushed, right?
There was a problem hiding this comment.
We need the push for buildx to be able to do multi-platform images.
That's the different between "local" (building the Docker image for the local host platform) and the "push" that actually do multi-platforms image (linux/amd64 and linux/arm64).
Good point for the plugin version, I will create a property to avoid the duplicate.
Thanks @jeanouii ! |
… properties Move docker-maven-plugin version and Java version to parent pom properties, parameterize the Dockerfile base image JDK version, and remove the unused build.sh script now superseded by Maven profiles. (apache#1701)
|
@jeanouii do you mind to do a new pass ? I think I addressed your comments. |
|
I think we can merge. Look good. I'll approve the PR |
2 participants
This closes #1701